Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test building with Sox on Windows #648

Closed
wants to merge 10 commits into from

Conversation

peterjc123
Copy link
Contributor

@peterjc123 peterjc123 commented May 17, 2020

Relates to #425

@peterjc123
Copy link
Contributor Author

Unit tests passed for both CPU and GPU with Sox on Windows. Marking it as ready, cc @vincentqb @mthrok.

@peterjc123 peterjc123 marked this pull request as ready for review May 17, 2020 17:40
Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the splendid job. Left some comments.

@@ -20,4 +25,6 @@ printf "Installing PyTorch with %s\n" "${cudatoolkit}"
conda install -y -c pytorch-nightly pytorch "${cudatoolkit}"

printf "* Installing torchaudio\n"
IS_CONDA=true python setup.py develop
curl --retry 3 https://s3.amazonaws.com/ossci-windows/torchaudio_deps.7z --output /tmp/torchaudio_deps.7z
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several questions regarding this

  • What does torchaudio_deps.7z contain?
  • Where is it from?
  • Does it include source or binary?
  • (if binary)
    • How was it built?
    • How to update it if dependencies should change in future?

Copy link
Contributor Author

@peterjc123 peterjc123 May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • What does torchaudio_deps.7z contain?

The header and the libraries of the third-party dependencies, like FLAC, libmad and libmp3lame.

  • Where is it from?

Built locally by myself.

  • Does it include source or binary?

Binary.

  • How was it built?

Generally speaking, I built them one by one manually according to the steps given by the project itself using VS 2019. However, some of the project support only very old format of the MSVC project, so I have to rewrite it using CMake.

  • How to update it if dependencies should change in future?

I hope that I could figure out the automatic build script for those projects. It is not easy because like I said in the previous question that I had to modify them a little bit. But I think it would be fine since some of the dependencies are not updated for a long time, like Sox and libmad. So for those old projects, maybe we could just download deps or maintain a fork. As for the active projects, like FLAC, we could just use the steps provided by the project itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not entirely sure if I would want to sign us up for this considering any update would have to be done by hand (however infrequent the updates may come).

Is there a way to contribute what we've done to make this dependency archive to those upstream dependencies?

extra_objects += [os.path.join(audio_path, 'third_party/lame/lib/mp3lame.lib')]
extra_objects += [os.path.join(audio_path, 'third_party/ogg/lib/ogg.lib')]
extra_objects += [os.path.join(audio_path, 'third_party/sox/lib/lpc10.lib')]
extra_objects += [os.path.join(audio_path, 'third_party/sox/lib/gsm.lib')]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #625, I am merging third party libraries into third_party/build/include and third_party/build/lib for simplicity. Could you adopt the same pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe I should wait for your PR? Or I just do that for Windows?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just do that for Windows.

namespace torch {
namespace audio {
namespace {
#ifdef _WIN32
int mkstemp(char* tmpl) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this your implementation, or taken from somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterjc123 peterjc123 mentioned this pull request May 18, 2020
11 tasks
@mthrok
Copy link
Collaborator

mthrok commented May 19, 2020

@seemethere

Could you take a look please?
This is a great addition to torchaudio, and the code itself looks reasonable to me.
But I do not have enough context around solution to provide pre-built binary for underlying codec libraries and the maintainability of that approach.
Is ossci-windows bucket managed by PyTorch org? (looking at this FAQ, it seems the case).

@peterjc123
Copy link
Contributor Author

Is ossci-windows bucket managed by PyTorch org? (looking at this FAQ, it seems the case).

Yes. I uploaded the torchaudio_deps.7z to that bucket.

the maintainability of that approach.

Just curious about the update frequency of the libraries. Looks like some of the libraries are not updated for a long time, like Sox, lame and mad.

@mthrok
Copy link
Collaborator

mthrok commented May 19, 2020

Just curious about the update frequency of the libraries. Looks like some of the libraries are not updated for a long time, like Sox, lame and mad.

Although chances are small, other potential cases for updating the dependencies for some unanticipated reason are removing one or adding a new one.

I think we want to make sure someone is able to reproduce the build steps you took in some form, just in case.

vc:
- 14
zip_keys:
- # [win]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this purposefully blank? Looks a bit confusing here without a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, and it should be a typo.

@facebook-github-bot
Copy link
Contributor

Hi @peterjc123!

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but we do not have a signature on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

mthrok pushed a commit to mthrok/audio that referenced this pull request Feb 26, 2021
…orials

Reorganize Production Usage section
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@mthrok mthrok closed this Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants